Refactored project structure #6

xstrengthofonex · 2020-01-19T17:21:09Z

Created basic project structure and moved production code into separate packages.

pkg/assert/assert.go

unional · 2020-01-19T18:14:57Z

Should we aim for one public function per file or per some grouping criteria?

For example, the internal/filter/filter.go seems redundant. If the filter.go contains multiple filters, internal/filter.go seems to make more sense (of course, this is subject to the general go file structuring rules).

This is just a general rule or guideline and we can break them as needed. Just want to bring the question up front. This is related to O/C at the file level which affects mergability.

xstrengthofonex · 2020-01-19T18:46:11Z

The main issue is that you should only have one package name per folder. If you put everything in the root internal folder, they would all be package internal, which would mess up all the namespaces.

unional · 2020-01-19T20:31:23Z

The main issue is that you should only have one package name per folder. If you put everything in the root internal folder, they would all be package internal, which would mess up all the namespaces.

Sure, in that case, may be internal/filter/filterOnCoStyle.go (or filter-on-co-style.go depends on convention)?

unional · 2020-01-19T23:13:10Z

Another comment is about CoStyle. Would it be too short? Would CollabStyle be a better name?

xstrengthofonex · 2020-01-20T01:10:08Z

CollabStyle is a good name. I can also change the filename to filter/on_collab_style.go

xstrengthofonex · 2020-01-20T02:33:39Z

How about changing the filter directory and package name to matcher? matcher is a more general name for the purpose of the package as opposed to filter which is more of an implementation strategy.

unional · 2020-01-20T03:10:55Z

How about changing the filter directory and package name to matcher? matcher is a more general name for the purpose of the package as opposed to filter which is more of an implementation strategy.

I wouldn't worry too much about it because we are still at the very beginning, things are pretty much in flux.

In fact, these logic can be changed drastically and we may ended up with a query/graphQL like code that there is one function that handles all the detail filtering and perform some additional match based on the result.

So, until then, I would say what we currently have is ok.

My two cents. 🌷

xstrengthofonex · 2020-01-20T03:53:33Z

I made the suggested changes. Don't worry about making suggestions, I'm using GoLand so refactoring is not a big problem.

akessner

I'm not sure I fully understand what 1 package per folder means, but it's not a big deal and I'll take your word on it for now.

But we do need to update the test names.

akessner · 2020-01-20T06:26:39Z

internal/filter/on_collab_style_test.go

+	"testing"
+)
+
+func TestFilterOnCoStyleReturnsEmpty(t *testing.T) {


We changed coStyle to colabStyle, but didn't update the test names.

Yeah, I forgot to update them. I'll do that.

akessner · 2020-01-20T06:27:46Z

internal/criteria/criteria.go

@@ -0,0 +1,11 @@
+package criteria
+
+// CollabStyle ..


Why the comment?

This is more out of annoyance. The linter in VScode keeps complaining when you don't put a comment. I don't know how to turn off the linter rule. I'm ok to delete them. I mostly use goland, so it's not much of an issue for me.

There got to be a way. I would say it is ok to keep them in this PR,
and the next PR add .vscode/settings.json to turn it off and remove these comments.
And in the travis turn on any linter check. I know that go compiler do some linting itself, but don't know about which linter the vscode extension based on.

I have looked, but wasn't able to find a way. I ended up just creating a snippet that added them automatically.

Are these comments supposed to be doc generating comments?

Ok, I see these are required. We should probably add godoc to the travis yaml.
https://golang.org/doc/effective_go.html#commentary

Since these comments are actually treated like code, they probably don't break the clean code "no comment" rules. And we should probably fill them in. Thoughts?
I'll raise this on the channel.

internal/filter/on_collab_style.go

akessner · 2020-01-20T06:41:58Z

internal/filter/on_collab_style.go

+
+// OnCollabStyle ..
+func OnCollabStyle(users []user.User, coStyle criteria.CollabStyle) (matchedUsers []user.User) {
+	matchedUsers = []user.User{}


A note for later.

If we use this style on all the match filters then we will end up looping through the entire user database multiple times.
We should make note to eventually only loop through the user DB once.

I agree with this

I also had a thought about this type of filter method. Because it's filtering out all the users that don't match with this specific criteria, if you were to use a chain of filters (in a functional way), the 'filtered out' users would not be present to be matched to other criterias. So, perhaps just returning the matching users might not be enough.

Instead we might want to consider using a new data structure that contains the user and a list of matches that have occurred. The function would be less of a filter and more of a matcher.

internal/user/user.go

akessner · 2020-01-20T07:02:04Z

Based on this: https://github.com/golang-standards/project-layout/blob/master/internal/README.md

Should we do internal/pckg/filter?
Btw, i prefer the package be named match, rather than filter.

akessner · 2020-01-20T07:03:23Z

Stupid phone, I did not mean to close the PR

akessner

Thanks for the insights.

akessner · 2020-01-20T15:00:08Z

Umm, I just found this issue.
I'll refrain from making suggestions based on the go-standards library from now on.
There seems to be a lot of controversy around it.
golang-standards/project-layout#10

Refactored project structure

9ceaf58

unional reviewed Jan 19, 2020

View reviewed changes

pkg/assert/assert.go Outdated Show resolved Hide resolved

xstrengthofonex added 2 commits January 20, 2020 12:41

Renamed coStyle to collabStyle

29dbd35

Moved assert package into internal

59452bd

unional previously approved these changes Jan 20, 2020

View reviewed changes

akessner requested changes Jan 20, 2020

View reviewed changes

akessner closed this Jan 20, 2020

akessner reopened this Jan 20, 2020

Renamed tests and created pkg and co_coders inside internal

757d5ff

xstrengthofonex dismissed unional’s stale review via 757d5ff January 20, 2020 13:32

akessner approved these changes Jan 20, 2020

View reviewed changes

xstrengthofonex merged commit 61df574 into master Jan 20, 2020

xstrengthofonex deleted the refactor-project-structure branch January 20, 2020 15:15

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactored project structure #6

Refactored project structure #6

xstrengthofonex commented Jan 19, 2020

unional commented Jan 19, 2020

xstrengthofonex commented Jan 19, 2020

unional commented Jan 19, 2020

unional commented Jan 19, 2020

xstrengthofonex commented Jan 20, 2020 •

edited

Loading

xstrengthofonex commented Jan 20, 2020 •

edited

Loading

unional commented Jan 20, 2020 •

edited

Loading

xstrengthofonex commented Jan 20, 2020 •

edited

Loading

akessner left a comment

akessner Jan 20, 2020

xstrengthofonex Jan 20, 2020

akessner Jan 20, 2020

xstrengthofonex Jan 20, 2020

unional Jan 20, 2020

xstrengthofonex Jan 20, 2020 •

edited

Loading

akessner Jan 20, 2020

akessner Jan 20, 2020 •

edited

Loading

akessner Jan 20, 2020

xstrengthofonex Jan 20, 2020

xstrengthofonex Jan 20, 2020 •

edited

Loading

akessner commented Jan 20, 2020 •

edited

Loading

akessner commented Jan 20, 2020

akessner left a comment

akessner commented Jan 20, 2020

Refactored project structure #6

Refactored project structure #6

Conversation

xstrengthofonex commented Jan 19, 2020

unional commented Jan 19, 2020

xstrengthofonex commented Jan 19, 2020

unional commented Jan 19, 2020

unional commented Jan 19, 2020

xstrengthofonex commented Jan 20, 2020 • edited Loading

xstrengthofonex commented Jan 20, 2020 • edited Loading

unional commented Jan 20, 2020 • edited Loading

xstrengthofonex commented Jan 20, 2020 • edited Loading

akessner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xstrengthofonex Jan 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akessner Jan 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xstrengthofonex Jan 20, 2020 • edited Loading

Choose a reason for hiding this comment

akessner commented Jan 20, 2020 • edited Loading

akessner commented Jan 20, 2020

akessner left a comment

Choose a reason for hiding this comment

akessner commented Jan 20, 2020

xstrengthofonex commented Jan 20, 2020 •

edited

Loading

xstrengthofonex commented Jan 20, 2020 •

edited

Loading

unional commented Jan 20, 2020 •

edited

Loading

xstrengthofonex commented Jan 20, 2020 •

edited

Loading

xstrengthofonex Jan 20, 2020 •

edited

Loading

akessner Jan 20, 2020 •

edited

Loading

xstrengthofonex Jan 20, 2020 •

edited

Loading

akessner commented Jan 20, 2020 •

edited

Loading